feat(connector)!: expose SUPPORT_DYN_VC_GFX_PROTOCOL early-cap flag for EGFX clients - #1237
feat(connector)!: expose SUPPORT_DYN_VC_GFX_PROTOCOL early-cap flag for EGFX clients#1237GlassOnTin wants to merge 1 commit into
Conversation
|
Reading through, this is the smallest possible fix and the doc-comment captures the foot-gun precisely. Without wiring a DvcClientProcessor for the Graphics channel, modern Windows servers stop sending legacy bitmap updates entirely and the desktop goes blank, which is exactly the GRD case in #1232 and the failure mode every downstream EGFX client hits. No conflict with #1098 (multitransport bootstrapping) on my read of the GCC block area, just two independent flags being OR'd in. Looks good from the server-side perspective too: any IronRDP server that already speaks EGFX (lamco-rdp-server, GRD) sees this flag and routes graphics through DVC as expected. |
27f3273 to
84ddffa
Compare
|
GlassOnTin Hi, could you rebase this? I'm trying to get this prioritized, and this would be a first step on the client side work I'm looking t. Thanks! |
84ddffa to
dbe5d82
Compare
|
Done — rebased onto current The only conflicts were in the test builders, where
|
|
Thank you! Benoît Cortier (@CBenoit) can you expedite this please? Thank you. My client issue will be posted later today. |
|
For reviewer context: I've opened #1472 as a sibling to this — a small |
…tionResult (#1488) Per [MS-RDPBCGR] 2.2.8.1.2, a client must not send fast-path input events unless the server advertised `INPUT_FLAG_FASTPATH_INPUT` or `INPUT_FLAG_FASTPATH_INPUT2` in its Input Capability Set. Today the connector discards the server's capability sets after Demand Active, so client code has no way to honour that requirement. This PR captures the server's Input capability flags during capabilities exchange, carries them through the `ConnectionFinalization`/`Finalized` activation states (so they are refreshed correctly across a Deactivation-Reactivation Sequence too), and surfaces them as `ConnectionResult::input_flags`. The session layer can then choose between fast-path and slow-path input per server. ### Motivation / real-world interop This is not theoretical: VirtualBox's VRDP server closes the connection outright on receiving a fast-path input PDU — its `VBox.log` reports ``` VRDP: Network packet length is incorrect 0x0004. Closing connection. ``` (a single fast-path scancode event is a 4-byte packet). VirtualBox never advertises fast-path input; its Demand Active offers `InputFlags(SCANCODES)` alone. mstsc and FreeRDP honour the negotiation and fall back to slow-path `TS_INPUT_PDU`s, which is why they work against VRDE. Haven (Android RDP client built on IronRDP) has been shipping this exact change as a vendored-connector patch since v5.86.1, with the slow-path fallback keyed off `ConnectionResult::input_flags`. Verified against a real VirtualBox 7.2.6 VRDE server: before the gate, the first arrow-key press killed the session with the log line above; with the gate, extended input sessions run clean, and a fast-path-capable server on the same host still takes the fast-path branch. (Discussed in #1158; this is the third and last piece Haven carries in its connector fork, alongside #1237 and #1472.) ### Changes - `connection_activation.rs`: capture `input_flags` from the `CapabilitySet::Input` in Server Demand Active (empty if absent); add the field to `ConnectionActivationState::{ConnectionFinalization, Finalized}`. - `connection.rs`: add `ConnectionResult::input_flags`, populated from the `Finalized` state. - Call sites in `ironrdp-client`, `ironrdp-web`, ffi, and the e2e test updated for the new variant field (all currently ignore it). ### Testing - Two new integration tests in `ironrdp-testsuite-core/tests/session/connection_activation.rs`: the fixture's Demand Active yields `SCANCODES | MOUSEX | UNICODE | FASTPATH_INPUT_2` in the `ConnectionFinalization` state, and a Demand Active with the Input capability stripped yields `InputFlags::empty()`. - `cargo check --workspace --all-targets`, `cargo clippy --workspace --all-targets`, and `cargo fmt --check` are clean; the 7 activation tests pass. Co-authored-by: GlassOnTin <GlassOnTin@users.noreply.github.com>
|
Adding |
…or EGFX clients Adds an opt-in `Config::support_dyn_vc_gfx_protocol` (default `false`). When set, `SUPPORT_DYN_VC_GFX_PROTOCOL` is OR'd into the client's early capability flags, so the server may negotiate `Microsoft::Windows::RDS::Graphics` for surface-based graphics instead of the legacy slow-path bitmap protocol. BREAKING CHANGE: `Config` is not `#[non_exhaustive]`, so the new field breaks downstream struct literals. Same shape as Devolutions#1501, which added `auto_reconnect_cookie` to this struct. The default is `false` on purpose. A client that advertises the bit without wiring an EGFX-capable `DvcClientProcessor` for that channel gets modern Windows servers to stop sending legacy bitmap updates altogether and route everything over EGFX, leaving the desktop blank — so the doc comment spells out the three steps to enable it properly. Coverage: - create_gcc_blocks unit tests (inline mod at file end, per Devolutions#1527) assert the bit is clear by default, set when enabled, and that the opt-in's symmetric difference against the default flag set is exactly that one bit — so it cannot quietly disturb the others. Verified to fail: gating on `if false` reddens two of the three. - testsuite-extra e2e runs a full client<->server connect with the bit advertised. Scope stated in the test: it asserts negotiation completes, not that the server acted on it, since ironrdp-server does not surface the client's early capability flags.
|
Thanks — both points taken. Rebased onto current On the source incompatibility: you're right, and I've stopped pretending otherwise — the PR is now marked breaking ( For what it's worth, #1501 added On the coverage: added both. Wire bit, in
These are checked to actually fail: replacing the End to end, in I want to be straight about that last one's scope rather than oversell it: it asserts negotiation completes with the bit set, not that the server acted on it.
|
dbe5d82 to
2ee58fb
Compare
The fork carried three patches. Two are now upstream — the server Input capability flags (#1488) and the lenient Server Font Map (#1506, which superseded our #1472 with a better-scoped fix: ours made FontPdu::decode lenient generally, and FontPdu is shared with Font List, so a malformed header-only *client* Font List would have decoded and advanced acceptor finalization). The third, the SUPPORT_DYN_VC_GFX_PROTOCOL early-capability flag, is still open as Devolutions/IronRDP#1237 and is the only thing our fork now adds on top of upstream master. What the released crates cost us: ironrdp-session 0.11.0 indexes the framebuffer from the decompressed buffer's length while bounds-checking only the destination rectangle, so a bitmap taller than its own dest rect writes past the end and panics. That is the #422 crash that closes Haven outright. Upstream fixed it in #1521 (2026-08-02) and no release carries it, so waiting for one means knowingly shipping an app-killing crash. Pinned to an immutable tag, haven-pin-20260803 = 2ee58fb1 = upstream e9e0bb49 + the #1237 patch. A tag rather than a bare rev because feat/egfx-early-cap-flag is an open PR that gets force-pushed on rebase, and the pinned object has to stay reachable. All thirteen ironrdp crates go through one [patch.crates-io] block: mixing git and crates.io links two copies of ironrdp-pdu and the types stop matching. Verified present in the pinned tree rather than assumed: - #1237 flag connector/src/connection.rs:1370 - #1488 input_flags on ConnectionResult - #1506 Font Map the FontMap dispatch arm, not FontPdu::decode - #1521 require_bitmap_data_size on all 7 apply_*_bitmap paths, including the bgr24 one the #422 tombstone points at COST: this forces the Rust toolchain 1.89.0 -> 1.94.1. Upstream raised its workspace rust-version in #1495 and #1521 landed after it, so there is no commit carrying the fix and the old floor; cargo refuses outright. rust-toolchain.toml and CI's `rustup default` move together. spice-kotlin keeps its own 1.89.0 pin — nothing there needs 1.94, and a directory-scoped toml lets them differ. Migration for four upstream breaking changes: - Config gains connection_type and support_dyn_vc_gfx_protocol (set true — Haven has the EGFX DVC processor wired, which is the foot-gun the upstream flag documents) - ConnectionActivationState::Finalized gains refresh_rect_support and suppress_output_support; reactivation reuses connect-time decisions, so the pattern takes `..` - ProcessorBuilder loses bulk_decompressor (#1255) — the processor always owns one now, so a reactivation no longer drops the decompression history a later compressed update refers back to - ActiveStageOutput gains SaveSessionInfo and AutoReconnectCookie (#1501). Both logged, not acted on: there is no session-resume path to spend the cookie on, and retaining a credential-equivalent token we would never use is not a trade worth making. 80/80 rust tests pass; all 3 ABIs rebuilt (the pin is inert until they are). arm64 librdp_transport.so 6,411,856 -> 6,185,896 bytes: the newer compiler and deps give back ~226 KB of what panic="unwind" cost. NOT verified: F-Droid is the authoritative build environment and a cargo *git* dependency has never been exercised there. Their recipe fetches from crates.io today and does `rustup default 1.85.0` (the crate-scoped toml overrides it, as it already does for 1.89). If the next F-Droid build fails on either, that is where to look first. panic="unwind" stays despite #1521. The tombstone frames were never symbolized, so attributing that crash to apply_bgr24_bitmap is inference; dropping the safety net to reclaim the size would be acting on it as if it were proven.
Currently,
early_capability_flagsin the GCC core data is built from a fixed set inconnection.rs. Clients that want to use the Graphics Pipeline Extension (MS-RDPEGFX) — by attaching aDvcClientProcessorforMicrosoft::Windows::RDS::Graphics— have no way to setSUPPORT_DYN_VC_GFX_PROTOCOLwithout forking the connector, and modern Windows servers won't open the EGFX channel unless the client advertises support.This PR adds an opt-in
Config.support_dyn_vc_gfx_protocol: bool(defaultfalse). When set, the flag is OR'd intoearly_capability_flagsalongside the existingWANT_32_BPP_SESSIONconditional. Existing consumers are unaffected; the doc comment includes a safety note that setting this without an EGFX implementation will cause Windows to stop sending legacy bitmap updates, leaving the desktop blank.Used downstream by Haven (an Android RDP/VNC client) which implements EGFX with ClearCodec + RemoteFxProgressive decoders; this lets us drop a vendored fork of
ironrdp-connectorwe currently carry just for this one flag.Default
falseto preserve current behaviour.cargo check --workspaceclean — six other in-treeConfig { … }builders updated with the default-false field.